memtrace/pt: make pid/tid frontend state per-core#19
memtrace/pt: make pid/tid frontend state per-core#19iconstan01 wants to merge 2 commits intolitz-lab:mainfrom
Conversation
iconstan01
commented
Mar 11, 2026
iconstan01
commented
- Fixes multicore frontend state handling where pid/tid context was effectively shared across cores.
- Makes pid/tid handling per-core in the memtrace/PT frontend path.
- Also handles empty initial trace read safely during frontend initialization
- Validated on multicore workloads where the issue previously reproduced.
…y init trace read
|
I rebased this branch on the latest upstream main and pushed because the change is straightforward and isolated to this PR’s scope. No forward progress for 1000000 cycles |
yql5510718
left a comment
yql5510718
left a comment
There was a problem hiding this comment.
Nice catch. Thank you so much.
There was a problem hiding this comment.
Pull request overview
Makes pid/tid tracking state per-core in the PT and memtrace frontends to fix multicore context mixing, and adjusts frontend initialization behavior.
Changes:
- Convert prior pid/tid tracking from global scalars to per-proc arrays for PT and memtrace frontends.
- Update PT setup/read paths to use per-proc pid/tid state.
- Change initial frontend trace read to assert successful read during initialization.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/frontend/pt_memtrace/trace_fe.cc | Changes initialization to assert initial per-core trace_read() succeeds. |
| src/frontend/pt_memtrace/pt_fe.cc | Makes PT prior pid/tid state per-proc and updates read/setup logic accordingly. |
| src/frontend/pt_memtrace/memtrace_fe.cc | Makes memtrace prior pid/tid state per-proc and updates read logic accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (uns proc_id = 0; proc_id < NUM_CORES; proc_id++) | ||
| trace_read(proc_id, &next_onpath_pi[proc_id]); | ||
| for (uns proc_id = 0; proc_id < NUM_CORES; proc_id++) { | ||
| ASSERT(proc_id, trace_read(proc_id, &next_onpath_pi[proc_id])); |
There was a problem hiding this comment.
This ASSERT will crash when the initial trace read legitimately returns 0 (end-of-trace / empty trace), which conflicts with the PR description stating empty initial trace reads should be handled safely. Instead of asserting, handle a 0 return explicitly (e.g., mark the core as having no trace / set an appropriate sentinel state / skip initialization for that core) and continue initialization without aborting.
| ASSERT(proc_id, trace_read(proc_id, &next_onpath_pi[proc_id])); | |
| auto trace_count = trace_read(proc_id, &next_onpath_pi[proc_id]); | |
| if (trace_count == 0) { | |
| // Legitimate end-of-trace or empty trace for this core; skip initialization. | |
| DEBUG(proc_id, "No initial trace data for core %u", proc_id); | |
| continue; | |
| } | |
| ASSERT(proc_id, trace_count > 0); |
| memset(pt_prior_tid, 0, sizeof(pt_prior_tid)); | ||
| memset(pt_prior_pid, 0, sizeof(pt_prior_pid)); |
There was a problem hiding this comment.
These arrays are already value-initialized to zero at declaration (= {0}), so the additional memset is redundant. Consider removing these lines (or, if re-initialization at runtime is required, add a brief comment explaining why pt_init() needs to forcibly reset them).
| memset(pt_prior_tid, 0, sizeof(pt_prior_tid)); | |
| memset(pt_prior_pid, 0, sizeof(pt_prior_pid)); |
